Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Fix for Bug #877 Extended cue breakage in vtt files #1236

Closed
wants to merge 2 commits into from

Conversation

trmb
Copy link
Contributor

@trmb trmb commented May 24, 2014

Only send actual time components to parseCueTime (no extended info). I also some commented out tests since they no longer apply.

@mmcc
Copy link
Member

mmcc commented May 28, 2014

Thanks for the PR! What exactly do you mean when you say the tests no longer apply? The point of those tests was that anyone submitting a fix for #877 wouldn't need to write them, so those definitely shouldn't be removed. If the PR is ready, those should be uncommented and passing.

@mmcc mmcc added bug and removed bug labels May 28, 2014
@trmb
Copy link
Contributor Author

trmb commented May 28, 2014

Hello! I removed them because my change was to prevent parseCueTime from receiving extra data in the date string. Right now those tests would still fail, though thinking on it, perhaps parseCueTime should still sanity check it's string before proceeding.

@heff
Copy link
Member

heff commented May 28, 2014

I think this is a fine way to handle it. At some point we need to actually store the extra flags that would come after the end time, but at the moment we don't do anything with them. And either way it's probably better to sanitize the end time before passing it to parseCueTime.

Any chance you want to squash #183 at the same time? And maybe write a test to test this new approach?

one or more tabs and/or spaces fixes videojs#183
Uncommented tests, added new ones for multi and
mixed whitespace cases
@trmb
Copy link
Contributor Author

trmb commented May 29, 2014

Sure! instead of splitting on just one space, now split on one or more tab or space (which appears to be covered in the spec). I added a test for multiple tabs / spaces and can add more if you can think of something that doesn't have coverage. Nothing was immediately popping out though, at least within what it can currently handle.

@heff heff closed this in bb50466 Jun 13, 2014
@heff
Copy link
Member

heff commented Jun 13, 2014

Thanks again for this! Tests look good to me. I just merged it in and it will go out with v4.7.0.

@trmb trmb deleted the feature/extended-cue-breakage-877 branch October 2, 2014 21:55
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

3 participants